[move-ide] On-hover improvements #19437
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
awelc
requested review from
cgswords,
damirka,
rvantonder,
dariorussi and
tnowacki
| #[derive(Copy, Clone, Debug)] | ||
| pub enum ModuleMemberKind { | ||
| Constant, | ||
| Function, | ||
| Struct, | ||
| Enum, | ||
| } | ||
|
|
||
| impl ModuleMemberKind { | ||
| pub fn case(self) -> NameCase { | ||
| match self { | ||
| ModuleMemberKind::Constant => NameCase::Constant, | ||
| ModuleMemberKind::Function => NameCase::Function, | ||
| ModuleMemberKind::Struct => NameCase::Struct, | ||
| ModuleMemberKind::Enum => NameCase::Enum, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Copy, Clone, Debug)] | ||
| pub enum NameCase { | ||
| Constant, | ||
| Function, | ||
| Struct, | ||
| Enum, | ||
| Module, | ||
| ModuleMemberAlias(ModuleMemberKind), | ||
| ModuleAlias, | ||
| Variable, | ||
| Address, | ||
| TypeParameter, | ||
| } | ||
|
|
||
| impl NameCase { | ||
| pub const fn name(&self) -> &'static str { | ||
| match self { | ||
| NameCase::Constant => "constant", | ||
| NameCase::Function => "function", | ||
| NameCase::Struct => "struct", | ||
| NameCase::Enum => "enum", | ||
| NameCase::Module => "module", | ||
| NameCase::ModuleMemberAlias(ModuleMemberKind::Function) => "function alias", | ||
| NameCase::ModuleMemberAlias(ModuleMemberKind::Constant) => "constant alias", | ||
| NameCase::ModuleMemberAlias(ModuleMemberKind::Struct) => "struct alias", | ||
| NameCase::ModuleMemberAlias(ModuleMemberKind::Enum) => "enum alias", | ||
| NameCase::ModuleAlias => "module alias", | ||
| NameCase::Variable => "variable", | ||
| NameCase::Address => "address", | ||
| NameCase::TypeParameter => "type parameter", | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if these should live in the AST. It might be worth making a new module called name_validation that has these plus the checking functions. Then at least all the code and definitions will still live together.
There was a problem hiding this comment.
+1 to these not being in the AST. You could stick them in expansion/mod.rs if you are really struggling for a place. But something like name_validation with all of the other various checking names (some of which might have been dragged into the AST already?) sounds like a great long term solution
There was a problem hiding this comment.
Done, though it required a bit more massaging as all these name validation functions take context as an argument to generate diagnostics
| // Implicit aliases for the Move Stdlib: | ||
| // use std::vector; | ||
| // use std::option::{Self, Option}; | ||
| pub const IMPLICIT_STD_MODULES: &[Symbol] = &[symbol!("option"), symbol!("vector")]; | ||
| pub const IMPLICIT_STD_MEMBERS: &[(Symbol, Symbol, ModuleMemberKind)] = &[( | ||
| symbol!("option"), | ||
| symbol!("Option"), | ||
| ModuleMemberKind::Struct, | ||
| )]; | ||
|
|
||
| // Implicit aliases for Sui mode: | ||
| // use sui::object::{Self, ID, UID}; | ||
| // use sui::transfer; | ||
| // use sui::tx_context::{Self, TxContext}; | ||
| pub const IMPLICIT_SUI_MODULES: &[Symbol] = &[ | ||
| symbol!("object"), | ||
| symbol!("transfer"), | ||
| symbol!("tx_context"), | ||
| ]; | ||
| pub const IMPLICIT_SUI_MEMBERS: &[(Symbol, Symbol, ModuleMemberKind)] = &[ | ||
| (symbol!("object"), symbol!("ID"), ModuleMemberKind::Struct), | ||
| (symbol!("object"), symbol!("UID"), ModuleMemberKind::Struct), | ||
| ( | ||
| symbol!("tx_context"), | ||
| symbol!("TxContext"), | ||
| ModuleMemberKind::Struct, | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
Consider implicit_imports.rs or similar for these definitions, because they don't really belong in the AST file.
There was a problem hiding this comment.
Yeah, a strange one.
We currently have Flavor defined in editions/mod.rs but maybe we need a flavor module. I think default implementations would make a lot of sense in that module, since the default imports are defined by the flavor
There was a problem hiding this comment.
Not sure if there is a concrete suggestion here on where to move it in this PR or it's something we are thinking about but could wait for a separate PR...
| @@ -90,10 +90,13 @@ use move_command_line_common::files::FileHash; | |||
| use move_compiler::{ | |||
There was a problem hiding this comment.
These changes all seem coherent, though I wonder if there is an opportunity to write a helper or two to simplify some of the code.
There was a problem hiding this comment.
I was thinking the same thing, but honestly I could not find a sensible way to helper-ize it as most additional are not easily separable from their enclosing context. Happy to follow suggestions, though!
| @@ -1,6 +1,6 @@ | |||
| module Symbols::M1 { | |||
|
|
|||
| struct SomeStruct has key, drop, store { | |||
There was a problem hiding this comment.
These changes are unrelated, but remove compilation errors which do not affect symbolication but are annoying when working with these Move files
| //************************************************************************************************** | ||
| // Valid names | ||
| //************************************************************************************************** | ||
|
|
There was a problem hiding this comment.
It's strange to me to change these all to Result<(),Box<Diagnostic>> from their original interface. Instead they should take CompilationEnv and retain their previous behavior.
There was a problem hiding this comment.
I was inspired by how parser does things but admittedly passing environment is simpler and leads to fewer code changes so ... done!
Description
This PR improves on several instance of on-hover.
vector::singletoninstead ofstd::vector::singleton) or both package and modules (for implicit datatypes, e.g.,Option, instead ofstd::option::Option)Test plan
All new and old tests must pass